Skip to content

[libcxx] Define _LIBCPP_HAS_C8RTOMB_MBRTOC8 for picolibc #152724

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

vhscampos
Copy link
Member

@vhscampos vhscampos commented Aug 8, 2025

Starting from picolibc 1.8.9, the char8_t related functions are provided.

This patch adds logic to detect the underlying picolibc version and define the _LIBCPP_HAS_C8RTOMB_MBRTOC8 macro accordingly.

Furthermore this patch changes the setting of the macro to true by default, but negates it in cases where it's known that the char8_t functions aren't implemented.

Starting from picolibc 1.8.9, the `char8_t` related functions are
provided.

This patch adds logic to detect the underlying picolibc version and
define the `_LIBCPP_HAS_C8RTOMB_MBRTOC8 macro` accordingly.
@vhscampos vhscampos requested a review from a team as a code owner August 8, 2025 13:38
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Aug 8, 2025
@llvmbot
Copy link
Member

llvmbot commented Aug 8, 2025

@llvm/pr-subscribers-libcxx

Author: Victor Campos (vhscampos)

Changes

Starting from picolibc 1.8.9, the char8_t related functions are provided.

This patch adds logic to detect the underlying picolibc version and define the _LIBCPP_HAS_C8RTOMB_MBRTOC8 macro accordingly.


Full diff: https://github.com/llvm/llvm-project/pull/152724.diff

2 Files Affected:

  • (modified) libcxx/include/__config (+6-2)
  • (modified) libcxx/include/__configuration/platform.h (+4)
diff --git a/libcxx/include/__config b/libcxx/include/__config
index 77a71b6cf1cae..ab87ed4787b84 100644
--- a/libcxx/include/__config
+++ b/libcxx/include/__config
@@ -1027,8 +1027,12 @@ typedef __char32_t char32_t;
 #    else
 #      define _LIBCPP_HAS_C8RTOMB_MBRTOC8 0
 #    endif
-#  else
-#    define _LIBCPP_HAS_C8RTOMB_MBRTOC8 0
+#  elif defined (_LIBCPP_PICOLIBC_PREREQ)
+#    if _LIBCPP_PICOLIBC_PREREQ(1, 8, 9) && defined(__cpp_char8_t)
+#      define _LIBCPP_HAS_C8RTOMB_MBRTOC8 1
+#    else
+#      define _LIBCPP_HAS_C8RTOMB_MBRTOC8 0
+#    endif
 #  endif
 
 // There are a handful of public standard library types that are intended to
diff --git a/libcxx/include/__configuration/platform.h b/libcxx/include/__configuration/platform.h
index f3c199dee172b..4f581a352038f 100644
--- a/libcxx/include/__configuration/platform.h
+++ b/libcxx/include/__configuration/platform.h
@@ -47,6 +47,10 @@
 //       user code. Move code paths that need _NEWLIB_VERSION to another customization mechanism.
 #if __has_include(<picolibc.h>)
 #  include <picolibc.h>
+#  define _LIBCPP_PICOLIBC_VERSION_INT(maj, min, patch) (maj * 10000 + min * 100 + patch)
+#  define _LIBCPP_PICOLIBC_PREREQ(maj, min, patch)                                                                     \
+    _LIBCPP_PICOLIBC_VERSION_INT(__PICOLIBC__, __PICOLIBC_MINOR__, __PICOLIBC_PATCHLEVEL__) >=                         \
+        _LIBCPP_PICOLIBC_VERSION_INT(maj, min, patch)
 #endif
 
 #ifndef __BYTE_ORDER__

Copy link

github-actions bot commented Aug 8, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

vhscampos added a commit to vhscampos/arm-toolchain that referenced this pull request Aug 8, 2025
PR llvm/llvm-project#152724 adds logic to define
the `_LIBCPP_HAS_C8RTOMB_MBRTOC8` macro depending on the picolibc
support for `char8_t` functions.

With this change we don't need a downstream patch anymore.
vhscampos added a commit to vhscampos/arm-toolchain that referenced this pull request Aug 8, 2025
PR llvm/llvm-project#152724 adds logic to define
the `_LIBCPP_HAS_C8RTOMB_MBRTOC8` macro depending on the picolibc
support for `char8_t` functions.

With this change we don't need a downstream patch anymore.
Copy link
Contributor

@philnik777 philnik777 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to refactor _LIBCPP_HAS_C8RTOMB_MBRTOC8 so that it's true unless we know it to be false (and also unconditionally make it true with Clang).

@vhscampos
Copy link
Member Author

@philnik777 I don't understand the two suggestions you made. Can you please give more details? Thank you

@philnik777
Copy link
Contributor

@philnik777 I don't understand the two suggestions you made. Can you please give more details? Thank you

_LIBCPP_HAS_C8RTOMB_MBRTOC8 is currently false if we don't know whether a library provides it or not. IMO we should assume it's available unless we know for a fact it's not. With Clang we don't care at all whether it's available, since we have _LIBCPP_USING_IF_EXISTS, so we can just always define _LIBCPP_HAS_C8RTOMB_MBRTOC8 to true in that case.

@vhscampos
Copy link
Member Author

I've refactored in a way that the macro is true "by default" as you suggested.

However, with regards to always setting it to true if compiling with clang, that causes a libcxx test to fail (link:

#if !defined(TEST_HAS_NO_C8RTOMB_MBRTOC8)
)

The value of macro TEST_HAS_NO_C8RTOMB_MBRTOC8 depends on _LIBCPP_HAS_C8RTOMB_MBRTOC8 (link:

# define TEST_HAS_NO_C8RTOMB_MBRTOC8
), so if we always set it to true for clang, this test fails if the functions aren't defined in the C library.

I couldn't think of a way to modify the test in order to have the macro true for clang.

@philnik777
Copy link
Contributor

I've refactored in a way that the macro is true "by default" as you suggested.

However, with regards to always setting it to true if compiling with clang, that causes a libcxx test to fail (link:

#if !defined(TEST_HAS_NO_C8RTOMB_MBRTOC8)

)

The value of macro TEST_HAS_NO_C8RTOMB_MBRTOC8 depends on _LIBCPP_HAS_C8RTOMB_MBRTOC8 (link:

# define TEST_HAS_NO_C8RTOMB_MBRTOC8

), so if we always set it to true for clang, this test fails if the functions aren't defined in the C library.

I couldn't think of a way to modify the test in order to have the macro true for clang.

We should probably just XFAIL the test on platforms that haven't fully implemented the interface. @ldionne any thoughts?

@vhscampos
Copy link
Member Author

Handling every case is brittle because I can't run the CI jobs locally, e.g. on AIX. Sorry for the high traffic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants